Skip to content

chore: sign user out if token is expired #109

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 18, 2025

Conversation

ethanndickson
Copy link
Member

@ethanndickson ethanndickson commented Mar 11, 2025

Closes #107.

When the menu bar icon is clicked, and the user is signed in, and the VPN is disabled, the app will check if the token is expired. If it is, the user will be signed out.

We could have checked this when the VPN is enabled, but the UX seemed worse, and the implementation would have been messy. We would have needed to sign the user out and show an error. Instead, we'll check for expiry in a scenario where the next user step would likely be an interaction that requires a session.
This approach also future-proofs for when functionality becomes usable without the VPN.

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@ethanndickson ethanndickson changed the title chore: sign user out on launch if token expired chore: sign user out on launch if token is expired Mar 11, 2025
@ethanndickson ethanndickson marked this pull request as ready for review March 11, 2025 07:08
@@ -94,6 +96,9 @@ class AppState: ObservableObject {
)
if hasSession {
_sessionToken = Published(initialValue: keychainGet(for: Keys.sessionToken))
if sessionToken == nil || sessionToken!.isEmpty == true {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can end up in this state if the user deleted it from the keychain themselves.

@ethanndickson ethanndickson force-pushed the ethan/sign-out-on-expiry branch from 15066a3 to 0617964 Compare March 11, 2025 07:12
@ethanndickson ethanndickson requested a review from ThomasK33 March 11, 2025 08:44
Comment on lines 59 to 75
// Sign out if token is expired
Task { @MainActor in
await state.handleTokenExpiry()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this run initially or periodically?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just once on launch. The token gets refreshed on use, so if the user toggles the VPN another week (by default) gets added to the expiry date. That means we're not covering the case where the user keeps the app open for a while without using it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Is this a case we should consider or is it unlikely for GA?

Copy link
Member Author

@ethanndickson ethanndickson Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd definitely like to be able to handle that case but, I don't think running it on a fixed interval is the right solution.
I realised it would be ideal if we could check the expiry if:

  • The user is signed in
  • The VPN is off
  • The menu bar icon is clicked

If the VPN is on, then the token has almost certainly been refreshed recently.
So, I did this with a quick commit to our FluidMenuBarExtra fork.
I think having these onAppear and onDisappear handlers is useful, in general, in case we ever want to refresh some state when the menu is opened.
For reference, the existing View.onAppear only runs when the view first appears for our menu bar window, as the view technically always exists, it's just not visible.

From pr desc:

We could have checked this when the VPN is enabled, but the UX seemed worse, and the implementation would have been messy. We would have needed to sign the user out and show an error.
This approach also future-proofs for when functionality becomes usable without the VPN

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks. I was thinking of the Tailscale experience, which shows me how long the device keys remain valid and when my login expires.

Can we potentially fetch information on when the token should expire (in the regular scenario) and display it in the UI? For example, if the token will expire in 24 hours, we conditionally show a red text that says "token expiring soon, please log in soon again" (or something comparable), but otherwise, it won't be shown.

Copy link
Member Author

@ethanndickson ethanndickson Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure a warning is useful, the session tokens have a pretty long default duration (7 days, and we only made it configurable late last year) and they refresh on use. If someone sees the warning by opening the menu, they were probably going to use the app anyway, which would refresh it.

As an aside, there's unfortunately no way to list cli-auth tokens, as there's no way to view information of tokens of type loginTypePassword via our API. Manually created tokens (coder tokens create) are of type loginTypetoken, which is all we support listing.

I think even if we did add a way to fetch those session durations, we have a HTTP middleware that refreshes on every request that requires a token, making the duration immediately either outdated or equal to the post-refresh duration.

@ethanndickson ethanndickson force-pushed the ethan/sign-out-on-expiry branch from dbd3eaf to cc50b5d Compare March 12, 2025 02:31
@ethanndickson ethanndickson changed the title chore: sign user out on launch if token is expired chore: sign user out if token is expired Mar 12, 2025
@ethanndickson ethanndickson self-assigned this Mar 12, 2025
@ethanndickson ethanndickson force-pushed the ethan/sign-out-on-expiry branch 2 times, most recently from 687f946 to bf95ba9 Compare March 12, 2025 05:50
@ethanndickson ethanndickson force-pushed the ethan/sign-out-on-expiry branch from bf95ba9 to c244963 Compare March 18, 2025 03:22
Copy link
Member Author

ethanndickson commented Mar 18, 2025

Merge activity

  • Mar 17, 11:36 PM EDT: A user started a stack merge that includes this pull request via Graphite.
  • Mar 17, 11:36 PM EDT: A user merged this pull request with Graphite.

@ethanndickson ethanndickson merged commit 9d93c0f into main Mar 18, 2025
4 checks passed
@ethanndickson ethanndickson deleted the ethan/sign-out-on-expiry branch March 18, 2025 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opening the app with an expired token should sign the user out
2 participants